Skip to content

Conversation

@silverwind
Copy link
Member

@silverwind silverwind commented Nov 3, 2025

A big step towards enabling strict mode in Typescript.

There was definitely a good share of potential bugs while refactoring this. When in doubt, I opted to keep the potentially broken behaviour. Notably, the DOMEvent type is gone, it was broken and we're better of with type assertions on e.target.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 3, 2025
@silverwind silverwind changed the title Enable strictNullChecks Enable TypeScript strictNullChecks Nov 3, 2025
@silverwind silverwind added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Nov 3, 2025
@lunny
Copy link
Member

lunny commented Nov 13, 2025

Too many ! in the class variables, looks ugly! could we assigned anything on the constructor to avoid them? Or we can disable strictPropertyInitialization.

@silverwind
Copy link
Member Author

silverwind commented Nov 13, 2025

Might be possible to move up the ! to the constructor, yes. Generally, thought there are many unavoidable ! for anything queried out of the DOM and I don't think there is a way to avoid those.

Also, I don't think we should relax any of the strict options, unless there is a clear benefit.

const refStyle = window.getComputedStyle(styleElement.value);
const refAltStyle = window.getComputedStyle(altStyleElement.value);
const refStyle = window.getComputedStyle(styleElement.value!);
const refAltStyle = window.getComputedStyle(altStyleElement.value!);
Copy link
Contributor

@wxiaoguang wxiaoguang Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's right to keep adding ! everywhere.

If you are refactoring, then it should follow the best practice and make the code readable / maintainable -- unless you will do quick following up changes to fix them in short time.

Just like the "jQuery removal", adding a lot ? only makes code difficult to read or maintain. At last I have to clean up them one by one to really finish the refactoring.


For this one, maybe the proper fix is to use useTemplateRef<T>

And a lot of other places also need a proper fix.

Copy link
Member Author

@silverwind silverwind Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is better to try to ensure stuff is non-nullable in first place, I will see what I can do, but some stuff like DOM queries are unfixable without major refactors that adds ifs everywhere and I think I will defer that to later and keep the !.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/frontend type/refactoring Existing code has been cleaned up. There should be no new functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants